Skip to content

feat: THIS keyword #1454

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 49 commits into from
May 9, 2025
Merged

feat: THIS keyword #1454

merged 49 commits into from
May 9, 2025

Conversation

abroooo
Copy link
Contributor

@abroooo abroooo commented Apr 6, 2025

Add THIS keyword support

Implements validation for the THIS keyword, which represents a pointer to the current FUNCTION_BLOCK instance.

Key points:

  • THIS can only be used within:
    • A FUNCTION_BLOCK implementation
    • METHODS belonging to that FUNCTION_BLOCK

Copy link
Member

@volsa volsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, mostly just some nits

Comment on lines 703 to 714
if function_context.linking_context.is_method()
&& self.index.find_pou(type_name).is_some_and(|it| it.is_function_block())
{
let alloca = self.llvm.builder.build_alloca(param_pointer.get_type(), "this");
self.llvm.builder.build_store(alloca, param_pointer);
index.associate_loaded_local_variable(type_name, "__THIS", alloca)?;
}
if function_context.linking_context.get_implementation_type() == &ImplementationType::FunctionBlock {
let alloca = self.llvm.builder.build_alloca(param_pointer.get_type(), "this");
self.llvm.builder.build_store(alloca, param_pointer);
index.associate_loaded_local_variable(type_name, "__THIS", alloca)?;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is actually neccessary because this is essentially an alias on the function argument %0, no? Specifically I would have assumed define void @FB_Test(%FB_Test* %0) to become define void @FB_Test(%FB_Test* %this) and then keep the codegen unchanged otherwise. However I wasn't able to find a quick solution for that without panicking so maybe theres another reason? Not a blocker though, can perhaps be improved in another PR

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commenting without reading the entire PR, but I would go with ignoring THIS all together in the codegen, the job of this is IMHO done with the resolver, the only exception to that is if we allow THIS^() as in calling the function block in the method but this could then be handled on its own

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we indeed allow THIS^(). Leaving this out of codegen completely doesnt work because then the variables don't get any association or did I get you wrong @ghaith?
For me what @volsa says makes sense. It just doesn't work for whatever reason 🙈

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They should still resolve to the local variable in the POU, so technically no need for this. it's just a marker to resolve the annotation to local instead of global, so the resolving scope during the annotation phase will be local only or similar, and then the annotation will be that of the local variable, when codegen comes along, it finds the annotation of the local variable on the expression and generates that, again theoretical I did not do a deep dive into the implementation.

Copy link
Contributor Author

@abroooo abroooo May 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed we leave it as is for now and I created an issue (#1475) so that we don't forget about this

@abroooo abroooo requested a review from volsa May 8, 2025 15:44
@abroooo abroooo merged commit 5f36a33 into master May 9, 2025
23 checks passed
@abroooo abroooo deleted the this_in_parser branch May 9, 2025 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants